Skip to content

Core: Add methods to work around IE active element bugs #1478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

scottgonzalez
Copy link
Member

Discussed with @tjvantoll and @arschmitz in IRC. These methods will not be documented and will not be supported; they're only for internal use.

jquery-archive/jquery-mobile#7927 should be updated if/when this lands.


// Support: IE9 - 10 only
// If the <body> is blurred, IE will switch windows, see #9420
if ( element && element.nodeName.toLowerCase() !== "body" ) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In mobile we also make sure that we avoid blurring the window. In fact, the code for retrieving the window and the document from the element is copied from the widget factory :)
https://github.com/jquery/jquery-mobile/pull/7927/files#diff-103715cf0e3db43f218c7235fd2a18fbR213

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that, it seemed really whacky. When are you ever passing a window?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the reason for that is that if you ever pass event.target or the context of an event handler you may end up passing in the window. I don't believe document.activeElement can ever be set to the window. I guess the basic assumption was that $( window ).blur() is unsafe in the same way as $( "body" ).blur() in that it results in the window losing focus. However, I've now tested $( window ).blur() as far back as IE6 and it does not seem to do anything, so I guess attempting to blur the window is safe.

I used the following test:

<!doctype html>
<html>
<head>
    <script src="http://code.jquery.com/jquery-1.11.1.js"></script>
    <script>
setTimeout( function() {
    $( "#console" ).text( "blurring window\n" );
    $( window ).blur();
}, 5000 );
    </script>
</head>
<body>
    <pre><code id="console"></code></pre>
    <input>
</body>
</html>

and I tried both with focus initially set to default and with focus immediately set on the input by my clicking on it.

@jzaefferer
Copy link
Member

You always pass this.document[ 0 ] to $.ui.safeActiveElement, that leads to two potential optimizations: Pass this.document and unwrap inside the method. Or add this method to $.Widget.prototype, removing the argument altogether, since you can just use this.document[ 0 ] directly.

These methods will not be documented and will not be supported; they're only for internal use.

Let's add source comments for that.

@scottgonzalez
Copy link
Member Author

You always pass this.document[ 0 ] to $.ui.safeActiveElement, that leads to two potential optimizations: Pass this.document and unwrap inside the method.

I wanted these methods to be as close to the intended APIs as possible. In the real APIs, we're always using DOM elements.

Or add this method to $.Widget.prototype, removing the argument altogether, since you can just use this.document[ 0 ] directly.

This will be used outside of widgets in jQuery Mobile. I should note that there are references to document.activeElement inside Effects Core, but I didn't want to add a dependency on UI Core and we've never had a bug report, so I figured I'd just leave them for now.

@scottgonzalez
Copy link
Member Author

Let's add source comments for that.

Done.

@jzaefferer
Copy link
Member

I just looked at the jQuery Mobile PR, but I didn't see any calls to the equivalent helpers outside of widgets. Both pagecontainer and popup are widgets. Are there other uses not covered by the PR? If so, this should be good to land as-is.

@scottgonzalez
Copy link
Member Author

I'm not sure, I was going by what @arschmitz said. I can say, though, that if we had already split UI Core into multiple files, I would have used this in Effects Core.

@arschmitz
Copy link
Member

Sorry the page container call used to be in navigation it was moved I forgot. The effects use seems valid though.

@tjvantoll
Copy link
Member

We should've done this a long time ago. 👍

@gabrielschulhof
Copy link

Actually, it might be simpler to override core's $.event.special.focus and $.event.special.blur. That would remove the need for introducing new API, and it would automagically work everywhere.

@gabrielschulhof
Copy link

Sorry, I just wanted to throw this in there, because I don't have the time right now to actually look in more detail, but I noticed that focus and blur are implemented in core as .trigger( "focus" ) and .trigger( "blur" ) and those events, in turn, are handled in $.event.special, so they are exposed to being overridden.

Also, core already has a private method safeActiveElement() which does the try {} catch(){} thing, so we need not duplicate that if we go the $.event.special route.

@gabrielschulhof
Copy link

We could override https://github.com/jquery/jquery/blob/master/src/event.js#L558-L576 and add the body-check (facepalm) into blur.

@scottgonzalez
Copy link
Member Author

Overriding $.event.special.focus in no way helps us with getting the active element. No matter what we do, we have to create a safeActiveElement() method.

Overriding $.event.special.blur is not an acceptable solution. Just because we don't want to trigger a blur on the document doesn't mean we should make it impossible to do so for all users.

@gabrielschulhof
Copy link

@scottgonzalez That makes sense.

@scottgonzalez scottgonzalez deleted the safe-active-element branch March 12, 2015 11:35
scottgonzalez added a commit that referenced this pull request Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants